Failing tests: placeholder gate rejects valid reordered human translations#25802
Merged
jkmassel merged 7 commits intoJul 18, 2026
Conversation
…nslations TranslationValidator compares a source's positional and sequential specifier views independently, so it only permits reordering when the source is ALREADY positional. A non-positional source (`%@ … %@`) whose translation reorders via positional specifiers (`%2$@ … %1$@`) is rejected — even though that is the standard, Apple-documented iOS reordering mechanism and is correct at runtime (`String(format:)` honors positional specifiers regardless of the source shape). The human-translation gate added for the String Catalog fold (`CatalogStrings.trusted_human`, `PluralStrings.human_forms_for`) inherits this, so it rejects and downgrades valid, currently-shipping human translations to machine/English. This is not hypothetical: the committed translated `.strings` contain 23 key-as-source strings across 34 locales (513 cells) that positionalize a bare-`%@` English source this way, e.g. ar "%@ of %@ used on your site" => "%1$@ من %2$@ على موقعك". Two failing tests pin the correct behavior: - translation_validator_test.rb — the root cause at the validator. - catalog_strings_helper_test.rb — the user-facing downgrade in the fold. The fix belongs in TranslationValidator (shared by AI, plurals, and regular human strings): treat a non-positional source as implicitly numbered 1..N by appearance order and accept a fully-positional candidate whose index→type map matches. Existing tests already keep genuine breakage (sequential flip, type/ count change) rejected, so a correct fix cannot just drop the distinction.
oguzkocer
force-pushed
the
catalog-strings-positional-gate-regression
branch
from
July 17, 2026 23:19
777cc6e to
92bf21f
Compare
3 tasks
…ource
TranslationValidator compared a source's positional and sequential specifier
views independently, so it only permitted reordering when the source was ALREADY
positional. A non-positional source (`%@ … %@`) whose translation reorders via
positional specifiers (`%2$@ … %1$@`) was rejected — even though that is the
standard, Apple-documented iOS mechanism and is correct at runtime, because
`String(format:)` honors positional specifiers regardless of the source's shape.
Normalize a purely-sequential signature into the implicit 1..N positional map its
bare specifiers already carry (`%@ %d` => `{1 => object, 2 => int}`) before
comparing, applied to both source and candidate. Once both sides are index→type
maps, `%@ - %@` and `%2$@ - %1$@` compare equal and the reorder is accepted.
Genuine breakage stays rejected: a sequential flip `%@: %d` => `%d : %@` yields
`{1 => object, 2 => int}` vs `{1 => int, 2 => object}` — a disagreement on which
index owns which type — and type/length/count changes are unaffected. A signature
that already uses positional specifiers, or one that mixes bare and positional
specifiers (malformed), is left unchanged, so mixed strings still require an exact
structural match.
Fixes the two failing tests added in the previous commit and propagates to all
three gate consumers (AI, plurals, regular human strings) via the shared
validator. All 7 fastlane/lanes suites pass (113 tests); rubocop clean.
…e cases Three currently-red tests documenting real defects found auditing the placeholder gate and the build-free coverage compare: - A candidate that binds the same positional index to two different types (%1$@ %1$d) is accepted: signature() collapses it last-wins, and this PR's implied_positional widened the reach to bare-specifier sources (%d), which regressed from reject to accept. - Dynamic field width/precision (%*d, %.*f) consume an extra int vararg the signature never counts, so %d and %*d reduce to the same signature. - coverage_gap conflates keys that differ only in argument type (%d days vs %@ days), masking a genuinely dropped key behind a same-prose sibling.
…ting type
signature() stored positional specifiers in a Hash keyed by index with plain
assignment, so a second reference to the same index (`%1$@` then `%1$d`)
silently overwrote the first, collapsing an object/int conflict to one token.
A candidate that references argument 1 as both an object and an int passes an
int to a `%@` conversion at runtime — a wrong-vararg read. This PR's
implied_positional widened the reach: a bare-specifier source like `%d` now
normalizes to {1 => int} and matched the collapsed candidate, regressing that
case from reject to accept.
Record a conflicting reuse as a distinct token so it can never equal a
well-formed source's single type, while still allowing a consistent reuse
(`%1$d … %1$d`, which legally reads one argument twice).
A `*` in a specifier's field width or precision (`%*d`, `%.*f`) consumes its own int argument before the value, so `%d` and `%*d` are not interchangeable. The FORMAT_SPECIFIER regex matched the `*` but signature() emitted no token for it, so the two reduced to the same signature and a translation could add or drop a dynamic-width/precision argument undetected — reading one vararg past what the call site supplies. Emit an int argument for each `*`, in appearance order before the value.
canonical() replaced every format specifier with one sentinel regardless of type, so keys differing only in argument type (`%d days` vs `%@ days`) collapsed to the same coverage cell — a genuinely-dropped key was masked by a same-prose sibling of a different type, defeating the same-basename overwrite regression the coverage gate exists to catch. Strip only the positional index (the sole difference between the genstrings source-form `%li` and the xcstringstool-normalized `%1$li` for the same source literal), so `%li` and `%1$li` still compare equal while `%d` and `%@` stay distinct.
… class Danger's RuboCop pass flagged two offenses from the previous commits: - Metrics/AbcSize: signature() exceeded the ABC limit once it gained the positional-conflict and dynamic-width handling. Extract record_specifier and add_positional so signature() is a thin loop again; behavior unchanged. - Style/OneClassPerFile: the coverage_gap test added a second top-level class to catalog_helper_test.rb. Fold it into the file's single test class, renamed CatalogHelperTest since it now covers both reword detection and the coverage compare.
jkmassel
merged commit Jul 18, 2026
8e9af7e
into
jkmassel/catalog-strings-translation
26 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Heads-up on a regression I hit reviewing #25713. This PR is failing tests only — no fix — so you can merge it and fix in place (or fold the fix into #25713 directly).
TranslationValidatorcompares a source's positional and sequential specifier views independently, so it only allows reordering when the source is already positional. A non-positional source (%@ … %@) whose translation reorders via positional specifiers (%2$@ … %1$@) is rejected — even though that's the standard, Apple-documented iOS reordering mechanism and is correct at runtime (String(format:)honors positional specifiers regardless of the source shape).The human-translation gate this PR adds (
CatalogStrings.trusted_human,PluralStrings.human_forms_for) inherits this and downgrades valid, currently-shipping human translations to machine/English. Real data: 23 key-as-source strings across 34 locales (513 shipping cells) do exactly this, e.g. ar"%@ of %@ used on your site"→"%1$@ من %2$@ على موقعك".Failing tests
translation_validator_test.rb→test_positionalizing_a_non_positional_source_to_reorder_is_allowed— the root cause at the validator.catalog_strings_helper_test.rb→test_reordered_human_translation_of_a_non_positional_source_is_kept_as_translated— the user-facing downgrade in the fold.Only these two are red; everything else passes.
Suggested fix
In
TranslationValidator(shared by AI, plurals, and regular human strings): treat a non-positional source as implicitly numbered1..Nby appearance order, and accept a fully-positional candidate whose index→(length:type) map matches that implied map. The existing tests already keep genuine breakage rejected — a sequential flip (%@: %d→%d : %@) and type/count changes — so the fix can't simply drop the positional/sequential distinction.